Update pipectl command to allow specifying labels of event#1419
Merged
pipecd-bot merged 2 commits intomasterfrom Jan 13, 2021
Merged
Update pipectl command to allow specifying labels of event#1419pipecd-bot merged 2 commits intomasterfrom
pipecd-bot merged 2 commits intomasterfrom
Conversation
Collaborator
Member
|
/lgtm |
nakabonne
reviewed
Jan 13, 2021
| message RegisterEventRequest { | ||
| string name = 1 [(validate.rules).string.min_len = 1]; | ||
| string data = 2 [(validate.rules).string.min_len = 1]; | ||
| map<string,string> labels = 3 [(validate.rules).map.keys.string.min_len = 1, (validate.rules).map.values.string.min_len = 1]; |
Member
There was a problem hiding this comment.
This validates just there is no empty value for a key? I just want to know this fails if no labels set. Sorry I'm lazy I didn't read the documentation for the validate.
Member
Author
There was a problem hiding this comment.
Yes. No labels case is fine. This checks against the empty string of the key, value.
Member
Author
There was a problem hiding this comment.
In case you want to know the generated code
/ Validate checks the field values on RegisterEventRequest with the rules
// defined in the proto definition for this message. If any rules are
// violated, an error is returned.
func (m *RegisterEventRequest) Validate() error {
if m == nil {
return nil
}
if utf8.RuneCountInString(m.GetName()) < 1 {
return RegisterEventRequestValidationError{
field: "Name",
reason: "value length must be at least 1 runes",
}
}
if utf8.RuneCountInString(m.GetData()) < 1 {
return RegisterEventRequestValidationError{
field: "Data",
reason: "value length must be at least 1 runes",
}
}
for key, val := range m.GetLabels() {
_ = val
if utf8.RuneCountInString(key) < 1 {
return RegisterEventRequestValidationError{
field: fmt.Sprintf("Labels[%v]", key),
reason: "value length must be at least 1 runes",
}
}
if utf8.RuneCountInString(val) < 1 {
return RegisterEventRequestValidationError{
field: fmt.Sprintf("Labels[%v]", key),
reason: "value length must be at least 1 runes",
}
}
}
return nil
}
Member
There was a problem hiding this comment.
That more makes sense! This generated code tell me how it works completely.
nakabonne
reviewed
Jan 13, 2021
| cmd.Flags().StringVar(&r.name, "name", r.name, "The name of event.") | ||
| cmd.Flags().StringVar(&r.data, "data", r.data, "The string value of event data.") | ||
| // TODO: Allow specifying event labels. | ||
| cmd.Flags().StringToStringVar(&r.labels, "labels", r.labels, "The list of labels for event. Format: key=value,key2=value2 .") |
Member
There was a problem hiding this comment.
nit: In that case, I feel like the period at the end of a sentence is unneeded.
Collaborator
Member
|
Thank you! |
Collaborator
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1393
Does this PR introduce a user-facing change?: